[Frame] hookify #2096
[Frame] hookify #2096
Conversation
sijad
force-pushed
the
hookify-frame
branch
4 times, most recently
from
f4753d4 to
1750f09
Compare
|
I think this is ready to be reviewed, I guess |
sijad
force-pushed
the
hookify-frame
branch
from
1750f09 to
99725d8
Compare
|
@AndrewMusgrave and @BPScott, would you have some time to look at this PR? Also, @sijad I think the codecov failure is legitimate. Check this report here, which shows all uncovered lines with dark red in the left margin: |
sijad
force-pushed
the
hookify-frame
branch
from
99725d8 to
280e3f5
Compare
|
Should code coverage issue get addressed in this PR? |
@sijad definitely. Unless there are extreme circumstances, we want 90% code coverage for a PR to merge. Please let us know if you need any help with how to test these changes, or in reading the code coverage report to know where to test. |
sijad
force-pushed
the
hookify-frame
branch
5 times, most recently
from
bb6a66e to
c080861
Compare
|
it's ready for review |
|
I don't have time to review this right now, I'll be able to Tuesday though. |
There was a problem hiding this comment.
LGTM other than the one question about skip to content. Thanks for taking this on 🙇 💯 Since this is such a foundation change we'll want a second pair of eyes cc/ @BPScott
| ); | ||
|
|
||
| const handleClick = useCallback(() => { | ||
| if (skipToContentTarget && skipToContentTarget.current) { |
There was a problem hiding this comment.
It looks like this previously only included logic to skip to the content target node. Why does it need logic to skip to the content target?
There was a problem hiding this comment.
TBH I'm not sure what it's even for, I'm not good with accessibility stuff, do you have any suggestion?
sijad
force-pushed
the
hookify-frame
branch
2 times, most recently
from
c8fba25 to
f081ff6
Compare
There was a problem hiding this comment.
Thank you for tackling this and for improving test coverage along the way!
I'm stupid busy atm and I'm not in a place where I can easily tophat this (yay bust company laptops) and I'm not that familiar with Frame's code and usage, so I'd also appreciate an extra pair of eyes. @dleroux would you be able to take a look when you get a moment?
I've dropped a few nitpick comments inline. The code looks good, aside from Andrew's queries about skipToTarget, and me wondering if we're handling ribbon resizing on browser resize as I can't see the EventListener for that anymore.
| </main> | ||
| <ToastManager toastMessages={toastMessages} /> | ||
| {globalRibbonMarkup} | ||
| <EventListener event="resize" handler={this.handleResize} /> |
There was a problem hiding this comment.
How are we handling this now? I can't spot an equivalent definition to catch window resizing
There was a problem hiding this comment.
I thought useMediaQuery does trigger resize for us here.
There was a problem hiding this comment.
I'm not familiar with how useMediaQuery does its thing. @AndrewMusgrave can you confirm?
There was a problem hiding this comment.
I've added resize back, nice catch!
There was a problem hiding this comment.
I'm not sure how often globalRibbonHeight should recalculated, I've added resize back for now, but useMediaQuery might be used as well
sijad
force-pushed
the
hookify-frame
branch
6 times, most recently
from
0963af0 to
a07a008
Compare
|
This issue has been inactive for 180 days and labeled with |
🤷♂️ |
ghost
closed this
WHY are these changes introduced?
related to #1995
WHAT is this pull request doing?
change
Frameto use react hooks🎩 checklist
README.mdwith documentation changes